Skip to content

perf: opt-in reflection cache via ApplicationContext; promote internal API to public#4

Merged
baal2000 merged 6 commits intoperformance/reflection-cachefrom
copilot/add-reflection-cache-settings
Mar 9, 2026
Merged

perf: opt-in reflection cache via ApplicationContext; promote internal API to public#4
baal2000 merged 6 commits intoperformance/reflection-cachefrom
copilot/add-reflection-cache-settings

Conversation

Copy link
Copy Markdown

Copilot AI commented Mar 8, 2026

Addresses the opt-in request from googleapis/google-api-dotnet-client#3118 review.

Motivation

The parent PR (googleapis#3118) caches PropertyInfo results to fix the DestroyScout finalizer pathology documented in #3112. The maintainer requested that caching be opt-in rather than always-on, citing concerns about unbounded cache growth and ConcurrentDictionary overhead.

Design decision: reuse ApplicationContext instead of a new class

Rather than introducing a new ReflectionCacheSettings class (adding permanent public API surface), this PR adds EnableReflectionCache to the existing ApplicationContext — the library's established pattern for app-wide startup settings (cf. ApplicationContext.RegisterLogger). This:

  • Adds zero new public types — a single property on an existing class
  • Is easier to obsolete later — if caching becomes the default, the property can be [Obsolete]-d without removing an entire class (consistent with how this library already handles API evolution, e.g. IClock.Now, GoogleClientSecrets.Load, ConfigurableMessageHandler.UnsuccessfulResponseHandlers)
  • Follows the library's own precedentApplicationContext is already referenced by ParameterUtils, RequestBuilder, ConfigurableMessageHandler, and BackOffHandler for exactly this "set once at startup" pattern

A note on the reviewer's concerns

For the record on the technical points raised in the review:

  • "400+ libraries in CI" scenario: The cache is per-process and keyed by Type. A CI testing libraries in separate processes would never accumulate all types in one cache. Even in the extreme single-process case, ~20K entries is trivial memory. The current no-cache behavior is actually "unbounded" in a worse way — it creates unbounded transient PropertyInfo arrays and IL stubs per request, which is exactly what triggers the GC/finalizer pathology in Performance: Reflection in ResumableUpload and ParameterUtils causes intermittent long response delays. googleapis/google-api-dotnet-client#3112.
  • ConcurrentDictionary bottleneck: GetOrAdd on a cache hit is a Volatile.Read — no lock is taken. Cost is ~20–50 ns. Without the cache, Type.GetProperties() + GetCustomAttribute() runs per request at 10–100 µs. The cache is strictly faster on every code path after the first call. This is the standard pattern recommended by Microsoft and used internally in ASP.NET Core, EF Core, and System.Text.Json for reflection caching.

Changes

File Change
ApplicationContext.cs Added EnableReflectionCache property (default false); updated class summary; Reset() now clears the flag
ReflectionCache.cs References ApplicationContext.EnableReflectionCache; public; extracted ComputeProperties; conditional cache path
PropertyWithAttribute.cs Promoted to public; comprehensive XML docs
ParameterUtils.cs InitParametersWithExpansion promoted internalpublic; stale InternalsVisibleTo remark removed
AssemblyInfo.cs Removed InternalsVisibleTo("Google.Apis", …)
Tests All references updated; tests for both cached/uncached modes; proper setup/teardown

Usage

// At application startup, before any API calls:
ApplicationContext.EnableReflectionCache = true;

Default behavior is unchanged — no caching, no new overhead.

Co-authored-by: baal2000 <22180333+baal2000@users.noreply.github.com>
Copilot AI changed the title [WIP] Add ReflectionCacheSettings class for opt-in caching Make reflection cache opt-in via ReflectionCacheSettings Mar 8, 2026
Copilot AI changed the title Make reflection cache opt-in via ReflectionCacheSettings Verify new tests pass with no regressions Mar 9, 2026
…ive XML docs

Co-authored-by: baal2000 <22180333+baal2000@users.noreply.github.com>
Copilot AI changed the title Verify new tests pass with no regressions Make ReflectionCache and PropertyWithAttribute public with comprehensive XML docs Mar 9, 2026
…ibute

Co-authored-by: baal2000 <22180333+baal2000@users.noreply.github.com>
Copilot AI changed the title Make ReflectionCache and PropertyWithAttribute public with comprehensive XML docs Make ReflectionCache and PropertyWithAttribute public with full XML docs Mar 9, 2026
…nsion to public

Co-authored-by: baal2000 <22180333+baal2000@users.noreply.github.com>
Copilot AI changed the title Make ReflectionCache and PropertyWithAttribute public with full XML docs Remove InternalsVisibleTo(Google.Apis) and make previously-internal API surface public Mar 9, 2026
@baal2000 baal2000 changed the title Remove InternalsVisibleTo(Google.Apis) and make previously-internal API surface public perf: make reflection cache opt-in via ApplicationContext and publicize cache API surface Mar 9, 2026
…lectionCacheSettings

Co-authored-by: baal2000 <22180333+baal2000@users.noreply.github.com>
Copilot AI changed the title perf: make reflection cache opt-in via ApplicationContext and publicize cache API surface Consolidate reflection cache opt-in into ApplicationContext; remove ReflectionCacheSettings Mar 9, 2026
@baal2000 baal2000 changed the title Consolidate reflection cache opt-in into ApplicationContext; remove ReflectionCacheSettings perf: opt-in reflection cache via ApplicationContext; promote internal API to public Mar 9, 2026
@baal2000 baal2000 marked this pull request as ready for review March 9, 2026 08:16
@baal2000 baal2000 merged commit df73cc0 into performance/reflection-cache Mar 9, 2026
@baal2000 baal2000 deleted the copilot/add-reflection-cache-settings branch March 9, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants